MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #132

carmenbianca · 2022-03-17T09:21:28Z

Duplicate of #117, but from a branch I can write to.

Fixes #103

legalsylvain · 2022-03-17T10:07:02Z

Hi @carmenbianca.
Thanks for your work !

Could you rebase on top of head OCA branch. There are 3 missing commits. (not a big deal, but it handles only the removal of .t2d.yml file.)
Don't you have to add keys in the test.yml of the current project ? if yes, you can merge that PR : [IMP] Add 10 / 11 / 12 branches in the tests coopiteasy/oca-addons-repo-template#2
I just tested on OCA/pos 12.0 branch. Here is the result : [12.0] DO NOT MERGE. try oca addons repo template support 10 11 12 coop it easy pos#764

The result is very red in the pre-commit checks. (https://github.com/OCA/pos/runs/5583735933?check_suite_focus=true)
the last merge in Travis, 11 days ago, was very green. (https://github.com/OCA/pos/runs/5439320107)

I'm afraid that changing the CI system on these "old" branches requires a lot of work here if you want all the branches to stay green. some choices :

keep travis and deploy manually on old branches. (not sure if it is possible, and in any case, it means continue to pay for Travis).
spend a lot of work on that current branch support-10-11-12.
assume that CI will be red, after merging that new CI, requiring some works for maintainers. (Optionaly, we could imagine that this year, the money saved on running travis could pay for some maintainer time to make it all green.)

@sbidoul : any point of view regarding that topic as a Board member ?

sbidoul · 2022-03-18T11:38:54Z

The result is very red in the pre-commit checks. (https://github.com/OCA/pos/runs/5583735933?check_suite_focus=true)

There are 3 kinds of faliure there:

a technical issue running flake8 (?) to be investigated, but probably fixable and not related to the repo
some pylint errors, which should not happen if the new pylint config is the same as MQTs ?
need to fix the website keys in manifest - that can and will be automated if/when we deploy the new pre-commit configs

sbidoul · 2022-03-18T11:40:38Z

So in principle, the only kind of errors should be related to invalid external dependencies where the new GitHub actions are stricter.

So I'd suggest digging a little bit more before deciding to keep travis.

legalsylvain · 2022-03-18T11:50:41Z

hi @sbidoul : Thanks for your point of view.

Let's go digging a little bit more. @carmenbianca : do you need some help ?
@sbidoul : could you merge some trivial / accepted PR to have the possibility to rebase that PR on top of up to date master branch ? (#115, #128, #130, #131 ...)

sbidoul · 2022-03-18T18:45:40Z

@legalsylvain I did some mergin'

Regarding impact on older branches here is a typical example of what needs to be done to fix dependencies: OCA/server-tools#2315. These changes are needed to make runboat work and changes made to please runboat are a prerequisite to please GitHub actions.

That said, runboat being green is not necessary for a merge, so... we may still decide to keep Travis if this proves too difficult.

carmenbianca · 2022-03-30T12:33:47Z

Made some progress.

OCA/pos#773 mostly passes, except some website keys were missing/wrong in manifests, and there's a single error being spotted by pylint-mandatory.

I chose to make flake8 run by pinning the Python version to 3.6 in the pre-commit config. This works for the following cases:

The CI, which I also pinned to Python 3.6 for version 11 and 12, and Python 2.7 for version 10.
Local development so long as python3.6 is available.

But it doesn't work for the tests in this repo. I'm not sure which knobs to twist to make that work, but I'll give it a look.

carmenbianca · 2022-03-30T12:53:35Z

Ready for review!

sbidoul · 2022-04-01T09:44:40Z

.github/workflows/test.yml

+        include:
+          - python-version: 2.7
+            odoo-version: 10.0
+        exclude:
+          - python-version: 3.8
+            odoo-version: 11.0
+          - python-version: 3.8
+            odoo-version: 12.0


This is the test of the template, why does it depend on the Odoo/python version combination ?

carmenbianca · 2022-04-05T12:30:55Z

@sbidoul I somehow made it impossible to reply to your review. The reason the template tests are pinned is because test_hooks_installable runs pre-commit install-hooks, which fails on mirrors-flake8 for Python 3.8 because Python 3.6 is specifically pinned for that item.

I could also fix this in the tests by pinning that test @skipif(odoo_version in ("11.0", "12.0") and python_version!="3.6") (or whatever the correct syntax is again).

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

The flake8 version used by these Odoo versions does not work with more recent versions of Python. Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

carmenbianca · 2022-04-29T09:29:07Z

@sbidoul @legalsylvain this PR needs a review before it becomes stale :)

sbidoul · 2022-04-29T15:08:19Z

Ok, lets merge this and see how it works. Thank you for your work on this @carmenbianca. Much appreciated!

AaronHForgeFlow · 2022-05-30T11:09:01Z

Thank you for the job! :)

Have any of you tested this for version 10.0?

I am stuck in the installation of test-requirements file:

Looking in indexes: https://wheelhouse.odoo-community.org/oca-simple-and-pypi
ERROR: Double requirement given: odoo<9.1a,>=9.0a (from -r test-requirements.txt (line 19)) (already in odoo<10.1dev,>=10.0 (from -r test-requirements.txt (line 18)), name='odoo')
Error: Process completed with exit code 1.

Thanks in advance.

sbidoul · 2022-05-30T12:14:42Z

@AaronHForgeFlow you probably have a mix of Odoo 9 and Odoo 10 addons in your repo ? Please check the versions in the manifests.

AaronHForgeFlow · 2022-05-30T12:50:46Z

@sbidoul You are right. The modules are installable=False but it seems that is not valid. I will remove those modules. Thank you!

sbidoul · 2022-05-30T13:07:37Z

Hm, if they are installable false they should be ignored.

sbidoul · 2022-05-30T13:11:16Z

That's weird, I'd be interested in more logs about that. Could you open an issue in oca/oca-ci ?

AaronHForgeFlow · 2022-05-30T13:14:56Z

I just checked twice and there was one module with installable=True and version 9.0.1.0.0. So there's no issue with oca/oca-ci . Sorry for the confusion here.

rousseldenis · 2022-12-06T13:50:23Z

src/.pre-commit-config.yaml.jinja

+{%- if odoo_version < 13 %}
+  {%- include "version-specific/mqt-compat/.pre-commit-config.yaml.jinja" %}
+
+{%- elif odoo_version < 14 %}


Is it possible that this line breaks updates for version < 13 as there is only one version-specific/13.0 folder... ?

@carmenbianca @sbidoul

Seems unlikely to me. It includes version-specific/mqt-compat, not version-specific/12.0.

It's about the line {%- elif odoo_version < 14 %}

That includes the {%- include "version-specific/%s/.pre-commit-config.yaml" % odoo_version %}part.

The line under mqt-compat 😃

Oooh I see; it's an elif.

But it seems to pass in that one for a 12.0 version.

Maybe the blank line ?

Maybe it's a string/float problem? It expects a float where there should be a string, or vice versa? Frankly I'm not sure. I wish I knew how to help.

You can try replacing odoo_version with odoo_version|float or odoo_version|int in these checks. Just in case you're passing the version as a string for some reason.

Also make sure you're using the latest copier.

legalsylvain mentioned this pull request Mar 17, 2022

[12.0] DO NOT MERGE. try oca addons repo template support 10 11 12 coop it easy OCA/pos#764

Closed

sbidoul mentioned this pull request Mar 18, 2022

[WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #117

Closed

carmenbianca force-pushed the support-10-11-12 branch 3 times, most recently from a6a90f6 to fffcb8f Compare March 30, 2022 09:36

carmenbianca force-pushed the support-10-11-12 branch from 526554b to 33efe3c Compare March 30, 2022 12:49

sbidoul reviewed Apr 1, 2022

View reviewed changes

carmenbianca force-pushed the support-10-11-12 branch from 0892ed2 to e78684a Compare April 8, 2022 09:42

sbidoul and others added 7 commits April 14, 2022 14:07

[WIP] MQT pre-commit + GitHub actions for 10, 11, 12

6757f72

Use extend functionality in mqt-compat/pylintrc

7a8bbd4

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

[FIX] Fix symbolic links to flake8, isort, prettier config

3408ea6

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

[FIX] Exclude some template files from prettier pre-commit

9a6cbfe

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

[FIX] Slightly adjust test to pass

6a35bba

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

[FIX] Hard-specify Python versions in pre-commit for Odoo <=12

ae12603

The flake8 version used by these Odoo versions does not work with more recent versions of Python. Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

[FIX] Use org_name and repo_website variables for MQT

add4eca

Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

carmenbianca force-pushed the support-10-11-12 branch from e78684a to add4eca Compare April 14, 2022 12:08

sbidoul merged commit 205a747 into OCA:master Apr 29, 2022

SirTakobi mentioned this pull request Jul 6, 2022

[WIP][10.0] test with GitHub actions and lint with pre-commit OCA/l10n-italy#2658

Closed

rousseldenis reviewed Dec 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #132

MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #132

carmenbianca commented Mar 17, 2022 •

edited

Loading

legalsylvain commented Mar 17, 2022 •

edited

Loading

sbidoul commented Mar 18, 2022 •

edited

Loading

sbidoul commented Mar 18, 2022

legalsylvain commented Mar 18, 2022

sbidoul commented Mar 18, 2022

carmenbianca commented Mar 30, 2022 •

edited

Loading

carmenbianca commented Mar 30, 2022

sbidoul Apr 1, 2022

carmenbianca commented Apr 5, 2022

carmenbianca commented Apr 29, 2022

sbidoul commented Apr 29, 2022

AaronHForgeFlow commented May 30, 2022 •

edited

Loading

sbidoul commented May 30, 2022

AaronHForgeFlow commented May 30, 2022

sbidoul commented May 30, 2022

sbidoul commented May 30, 2022

AaronHForgeFlow commented May 30, 2022

rousseldenis Dec 6, 2022

rousseldenis Dec 6, 2022

carmenbianca Dec 6, 2022

rousseldenis Dec 6, 2022

rousseldenis Dec 6, 2022

carmenbianca Dec 7, 2022

yajo Jan 10, 2023

MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #132

MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #132

Conversation

carmenbianca commented Mar 17, 2022 • edited Loading

legalsylvain commented Mar 17, 2022 • edited Loading

sbidoul commented Mar 18, 2022 • edited Loading

sbidoul commented Mar 18, 2022

legalsylvain commented Mar 18, 2022

sbidoul commented Mar 18, 2022

carmenbianca commented Mar 30, 2022 • edited Loading

carmenbianca commented Mar 30, 2022

Choose a reason for hiding this comment

carmenbianca commented Apr 5, 2022

carmenbianca commented Apr 29, 2022

sbidoul commented Apr 29, 2022

AaronHForgeFlow commented May 30, 2022 • edited Loading

sbidoul commented May 30, 2022

AaronHForgeFlow commented May 30, 2022

sbidoul commented May 30, 2022

sbidoul commented May 30, 2022

AaronHForgeFlow commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmenbianca commented Mar 17, 2022 •

edited

Loading

legalsylvain commented Mar 17, 2022 •

edited

Loading

sbidoul commented Mar 18, 2022 •

edited

Loading

carmenbianca commented Mar 30, 2022 •

edited

Loading

AaronHForgeFlow commented May 30, 2022 •

edited

Loading